Skip to content

Update retained size without causing lock congestion#13213

Merged
sopel39 merged 1 commit intotrinodb:masterfrom
starburstdata:ks/lock_fix
Jul 19, 2022
Merged

Update retained size without causing lock congestion#13213
sopel39 merged 1 commit intotrinodb:masterfrom
starburstdata:ks/lock_fix

Conversation

@sopel39
Copy link
Copy Markdown
Member

@sopel39 sopel39 commented Jul 18, 2022 •
edited
Loading

Description

Is this change a fix, improvement, new feature, refactoring, or other?

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

How would you describe this change to a non-technical end user or system administrator?

Related issues, pull requests, and links

Documentation

( ) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

Fixes: #13212

@cla-bot cla-bot bot added the cla-signed label Jul 18, 2022
@sopel39
Copy link
Copy Markdown
Member Author

sopel39 commented Jul 18, 2022

cc @martint

losipiuk
losipiuk reviewed Jul 18, 2022
View reviewed changes
Copy link
Copy Markdown
Member

@losipiuk losipiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. Thanks

@sopel39 sopel39 force-pushed the ks/lock_fix branch 3 times, most recently from 768ef6d to ee4a8a7 Compare July 18, 2022 15:01
arhimondr
arhimondr approved these changes Jul 18, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@arhimondr arhimondr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While separating locks is a step in right direction I'm not sure how much this will reduce lock contention in practice. The MemoryContext#setBytes is globally synchronized. And the other code withing the DirectExchangeClient doesn't seem to be doing much (though I could be wrong). After this change the contention will likely move to MemoryContext#setBytes. I think a good question to ask is why poolPage / addPages are even highly contended? Is there a small page problem?

@sopel39
Copy link
Copy Markdown
Member Author

sopel39 commented Jul 19, 2022

After this change the contention will likely move to MemoryContext#setBytes.

It will probably move to io.trino.memory.MemoryPool#reserve, which is shared across all queries on node. If we have problem with congestion on that level, then we need to fix it holistically, because a lot of operators are updating MemoryContext. Maybe MemoryPool and QueryContext should be lock free.

@sopel39 sopel39 merged commit 49003d5 into trinodb:master Jul 19, 2022
@sopel39 sopel39 deleted the ks/lock_fix branch July 19, 2022 12:15
@sopel39
Copy link
Copy Markdown
Member Author

sopel39 commented Jul 19, 2022

RN proposal

General
* Reduce worker lock congestion. ({issue}`13213`)

#13213

@github-actions github-actions bot added this to the 391 milestone Jul 19, 2022
@colebow colebow mentioned this pull request Jul 19, 2022
Merged
@colebow
Copy link
Copy Markdown
Member

colebow commented Jul 19, 2022

Could you go into a little more detail on what lock congestion is? I think we'll need to explain things further in the release notes and tell our users why they should care about this.

@sopel39
Copy link
Copy Markdown
Member Author

sopel39 commented Jul 20, 2022

Could you go into a little more detail on what lock congestion is? I think we'll need to explain things further in the release notes and tell our users why they should care about this.

We are still assessing how big of an impact it is. In any case calling setBytes within synchronization section should have been improved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@losipiuk losipiuk losipiuk left review comments

@findepi findepi Awaiting requested review from findepi

@raunaqmorarka raunaqmorarka Awaiting requested review from raunaqmorarka

+1 more reviewer

@arhimondr arhimondr arhimondr approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

cla-signed performance

Milestone

391

Development

Successfully merging this pull request may close these issues.

Lock congestion in DirectExchangeClient

5 participants

@sopel39 @colebow @losipiuk @arhimondr @raunaqmorarka